sync::mpsc: prevent double free on `Drop` #139553

petrosagg · 2025-04-08T19:42:41Z

This PR is fixing a regression introduced by #121646 that can lead to a double free when dropping the channel.

The details of the bug can be found in the corresponding crossbeam PR crossbeam-rs/crossbeam#1187

rustbot · 2025-04-08T19:42:45Z

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

taiki-e · 2025-04-08T20:57:57Z

cc @ibraheemdev

tgross35 · 2025-04-08T23:20:13Z

Is it possible add a test reproducing the failure? Ideally something ./x miri library/std ... flags without this fix so we catch any regressions.

petrosagg · 2025-04-09T08:56:54Z

I can try to find a miri seed that leads to the problematic thread scheduling but I'm worried that if the code of the channel implementation changes or the version of miri changes the tested schedule will change as well and the test won't be testing what we think it does anymore.

petrosagg · 2025-04-09T08:57:12Z

I accidentally clicked the close button, sorry!

tgross35 · 2025-04-09T17:30:57Z

I think it's fine to just include an isolated test that makes conditions for this problem favorable, extra good if's reasonably fast to provide a seed+sha for which Miri shows the problem locally.

Cc @RalfJung

petrosagg · 2025-04-09T22:09:19Z

@tgross35 I have been running Miri with the nightly-2025-04-08 toolchain using the command line MIRIFLAGS="-Zmiri-many-seeds=0..1000000000" cargo miri run on the following program but no luck with a specific seed yet.

use std::sync::mpsc::channel;
use std::thread;

fn main() {
    let (tx, rx) = channel::<i32>();
    let tx2 = tx.clone();

    let t1 = thread::spawn(move || {
        let _ = tx.send(1);
    });

    tx2.send(1).unwrap();
    drop(rx);
    let _ = t1.join();
}

It is fairly easy to reproduce manually by adding a sleep at the right place and running the test of this commit adapter to std's channels crossbeam-rs/crossbeam@00dcea6.

I will leave Miri running overnight and report back with any interesting seeds.

RalfJung · 2025-04-10T06:00:03Z

Does the bug reproduce in Miri with the sleep? If it's easy to reproduce outside Miri, the same code show also be quite likely to reproduce it inside Miri.

Maybe we could have a -Zmiri-preemption-rate=0 test with a #[cfg(miri)] yield_now() in the right place inside the channel. However that yield_now would then always be present in the Miri sysroot version of this code...

petrosagg · 2025-04-10T08:46:16Z

After leaving Miri running overnight it didn't find a suitable seed that produces the interleaving that causes the double free. I'm not sure why this interleaving is hard to reach.

I pushed a commit here petrosagg@79372e9 that adds a sleep to make the specific thread interleaving favorable and the problem reproduces with both normal tests and Miri. You can reproduce it with either ./x test src/tools/miri --test-args tests/pass/gh_139553.rs or ./x test library/std --test-args gh_139553

The normal test will print free(): double free detected in tcache 2 and Miri outputs the following:

error: Undefined Behavior: out-of-bounds pointer use: alloc4180 has been freed, so this pointer is dangling
  --> /home/petrosagg/projects/rust/library/alloc/src/boxed.rs:1154:9
   |
LL |         Box(unsafe { Unique::new_unchecked(raw) }, alloc)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: alloc4180 has been freed, so this pointer is dangling
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc4180 was allocated here:
  --> tests/pass/gh_139553.rs:9:44
   |
LL |     let t1 = thread::spawn(move || assert!(s1.send(42).is_err()));
   |                                            ^^^^^^^^^^^
help: alloc4180 was deallocated here:
  --> tests/pass/gh_139553.rs:18:5
   |
LL |     drop(r);
   |     ^^^^^^^
   = note: BACKTRACE (of the first span):
   = note: inside `std::boxed::Box::<std::sync::mpmc::list::Block<u64>>::from_raw_in` at /home/petrosagg/projects/rust/library/alloc/src/boxed.rs:1154:9: 1154:58
   = note: inside `std::boxed::Box::<std::sync::mpmc::list::Block<u64>>::from_raw` at /home/petrosagg/projects/rust/library/alloc/src/boxed.rs:1045:18: 1045:48
   = note: inside `<std::sync::mpmc::list::Channel<u64> as std::ops::Drop>::drop` at /home/petrosagg/projects/rust/library/std/src/sync/mpmc/list.rs:654:22: 654:42
   = note: inside `std::ptr::drop_in_place::<std::sync::mpmc::list::Channel<u64>> - shim(Some(std::sync::mpmc::list::Channel<u64>))` at /home/petrosagg/projects/rust/library/core/src/ptr/mod.rs:523:1: 523:56
   = note: inside `std::ptr::drop_in_place::<std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<u64>>> - shim(Some(std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<u64>>))` at /home/petrosagg/projects/rust/library/core/src/ptr/mod.rs:523:1: 523:56
   = note: inside `std::ptr::drop_in_place::<std::boxed::Box<std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<u64>>>> - shim(Some(std::boxed::Box<std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<u64>>>))` at /home/petrosagg/projects/rust/library/core/src/ptr/mod.rs:523:1: 523:56
   = note: inside `std::mem::drop::<std::boxed::Box<std::sync::mpmc::counter::Counter<std::sync::mpmc::list::Channel<u64>>>>` at /home/petrosagg/projects/rust/library/core/src/mem/mod.rs:935:24: 935:25
   = note: inside `std::sync::mpmc::counter::Sender::<std::sync::mpmc::list::Channel<u64>>::release::<{closure@<std::sync::mpmc::Sender<u64> as std::ops::Drop>::drop::{closure#1}}>` at /home/petrosagg/projects/rust/library/std/src/sync/mpmc/counter.rs:65:17: 65:61
   = note: inside `<std::sync::mpmc::Sender<u64> as std::ops::Drop>::drop` at /home/petrosagg/projects/rust/library/std/src/sync/mpmc/mod.rs:633:45: 633:85
   = note: inside `std::ptr::drop_in_place::<std::sync::mpmc::Sender<u64>> - shim(Some(std::sync::mpmc::Sender<u64>))` at /home/petrosagg/projects/rust/library/core/src/ptr/mod.rs:523:1: 523:56
   = note: inside `std::ptr::drop_in_place::<std::sync::mpsc::Sender<u64>> - shim(Some(std::sync::mpsc::Sender<u64>))` at /home/petrosagg/projects/rust/library/core/src/ptr/mod.rs:523:1: 523:56
note: inside `main`
  --> tests/pass/gh_139553.rs:20:1
   |
LL | }
   | ^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

RalfJung · 2025-04-10T09:08:20Z

I pushed a commit here petrosagg@79372e9 that adds a sleep to make the specific thread interleaving favorable

If you make this #[cfg(miri)] thread::yield_now();, and also run Miri with MIRIFLAGS="-Zmiri-preemption-rate=0", can Miri still reproduce the issue? That might need more targeted yields in more places, not sure.

petrosagg · 2025-04-10T09:21:56Z

I will try reproducing with just yield_now() now and report back

petrosagg · 2025-04-10T12:20:03Z

I'm not able to reproduce it even with a yield_now() call at the place where I put the sleep in the manual repro. I tried various combinations of parameters and seeds but nothing seems to trigger it.

rustbot · 2025-04-11T10:46:30Z

The Miri subtree was changed

cc @rust-lang/miri

petrosagg · 2025-04-11T10:46:32Z

I managed to repro it with Miri and simple thread yields! I think the default compare_and_exchange_weak failure rate was the reason I couldn't get it to reproduce before. I have now updated the PR with two commits. The first adds the reproducer without the fix and should fail with ./x test src/tools/miri --test-args tests/pass/concurrency/issue139553.rs. The second commit adds the fix and makes the test pass.

cc @RalfJung @tgross35

saethlin · 2025-04-11T11:11:59Z

I think the default compare_and_exchange_weak failure rate was the reason I couldn't get it to reproduce before.

Is it your estimation that the default failure rate is too high, or is it too low?

petrosagg · 2025-04-11T11:48:29Z

Is it your estimation that the default failure rate is too high, or is it too low?

Too high. My test is basically baking a thread schedule (by calling yield_now()) that only triggers the bug if the compare_and_exchange_weak in start_send doesn't spuriously fail. So any non-zero probability of spurious failure will mess up the intended schedule.

That said, the overnight test looking for seed values had only one yield_now() call whereas the test that I have committed now has one more in the test itself. That means that I had to be extra lucky for the random schedule to pick the appropriate points for yielding.

RalfJung · 2025-04-11T11:56:52Z

src/tools/miri/tests/pass/concurrency/issue139553.rs

We have tests/pass/issues for tests testing specific issues. Also please put a comment in the file explaining the context of the test.

Oh, I totally missed that directory. I moved the test there and added detailed comments explaining the particular interleaving and the way the double free is caused.

RalfJung · 2025-04-11T11:58:35Z

library/std/src/sync/mpmc/list.rs

@@ -213,6 +213,8 @@ impl<T> Channel<T> {
                    .compare_exchange(block, new, Ordering::Release, Ordering::Relaxed)
                    .is_ok()
                {
+                    #[cfg(miri)]
+                    crate::thread::yield_now();


The problem with this is that this will then always be present when Miri executes this code, even outside the test. Ideally we'd only enable this on our CI. This will require some new flag though...

Also there should be a comment explaining the point of this.

Added a comment explaining why this point is interesting and a pointer to the test that uses it

Thanks!

I guess now the main question is whether it's worth leaving in this yield for all Miri users or whether it should be somehow only applied for the test suite. It probably doesn't harm to always have it...

I would also lean towards leaving there. It does yield at a very interesting point of the channel in general so having it there might catch future regressions sooner

library/std/src/sync/mpmc/list.rs

Signed-off-by: Petros Angelatos <[email protected]>

This PR is fixing a regression introduced by rust-lang#121646 that can lead to a double free when dropping the channel. The details of the bug can be found in the corresponding crossbeam PR crossbeam-rs/crossbeam#1187 Signed-off-by: Petros Angelatos <[email protected]>

RalfJung · 2025-04-17T06:11:54Z

Miri changes LGTM. @tgross35 I am going to take this as an "r=me for library changes" from your side.

@bors r=RalfJung,tgross35

bors · 2025-04-17T06:11:57Z

📌 Commit b9e2ac5 has been approved by RalfJung,tgross35

It is now in the queue for this repository.

…alfJung,tgross35 sync::mpsc: prevent double free on `Drop` This PR is fixing a regression introduced by rust-lang#121646 that can lead to a double free when dropping the channel. The details of the bug can be found in the corresponding crossbeam PR crossbeam-rs/crossbeam#1187

ibraheemdev · 2025-04-17T10:15:23Z

Sorry I'm very busy as of late, but from a cursory look this seems good.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#138934 (support config extensions) - rust-lang#139114 (Implement `pin!()` using `super let`) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

…alfJung,tgross35 sync::mpsc: prevent double free on `Drop` This PR is fixing a regression introduced by rust-lang#121646 that can lead to a double free when dropping the channel. The details of the bug can be found in the corresponding crossbeam PR crossbeam-rs/crossbeam#1187

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#138934 (support config extensions) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#139553 - petrosagg:channel-double-free, r=RalfJung,tgross35 sync::mpsc: prevent double free on `Drop` This PR is fixing a regression introduced by rust-lang#121646 that can lead to a double free when dropping the channel. The details of the bug can be found in the corresponding crossbeam PR crossbeam-rs/crossbeam#1187

[beta] backports - Fix 2024 edition doctest panic output rust-lang#139328 - make `Arguments::as_statically_known_str` doc(hidden) rust-lang#139389 - Revert "Deduplicate template parameter creation" rust-lang#139878 - sync::mpsc: prevent double free on `Drop` rust-lang#139553 r? cuviper

…alfJung,tgross35 sync::mpsc: prevent double free on `Drop` This PR is fixing a regression introduced by rust-lang#121646 that can lead to a double free when dropping the channel. The details of the bug can be found in the corresponding crossbeam PR crossbeam-rs/crossbeam#1187

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned jhpratt Apr 8, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 8, 2025

taiki-e approved these changes Apr 8, 2025

View reviewed changes

petrosagg closed this Apr 9, 2025

petrosagg reopened this Apr 9, 2025

petrosagg force-pushed the channel-double-free branch from 8939b53 to 19219f5 Compare April 11, 2025 10:46

RalfJung reviewed Apr 11, 2025

View reviewed changes

petrosagg force-pushed the channel-double-free branch from 19219f5 to 63fafe3 Compare April 11, 2025 12:19

sync::mpsc: add miri reproducer of double free

9eb6a54

Signed-off-by: Petros Angelatos <[email protected]>

petrosagg force-pushed the channel-double-free branch from 63fafe3 to 72c9a47 Compare April 11, 2025 12:29

petrosagg force-pushed the channel-double-free branch from 72c9a47 to b9e2ac5 Compare April 11, 2025 12:33

rustbot assigned RalfJung and unassigned jhpratt Apr 16, 2025

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 16, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2025

matthiaskrgr mentioned this pull request Apr 17, 2025

Rollup of 9 pull requests #139957

Closed

matthiaskrgr mentioned this pull request Apr 17, 2025

Rollup of 8 pull requests #139980

Closed

Amanieu added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 17, 2025

matthiaskrgr mentioned this pull request Apr 18, 2025

Rollup of 7 pull requests #139996

Merged

bors merged commit 5e8bc7f into rust-lang:master Apr 18, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone Apr 18, 2025

cuviper mentioned this pull request Apr 18, 2025

[beta] backports #139988

Merged

cuviper modified the milestones: 1.88.0, 1.87.0 Apr 18, 2025

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync::mpsc: prevent double free on `Drop` #139553

sync::mpsc: prevent double free on `Drop` #139553

petrosagg commented Apr 8, 2025

rustbot commented Apr 8, 2025

taiki-e commented Apr 8, 2025

tgross35 commented Apr 8, 2025

petrosagg commented Apr 9, 2025

petrosagg commented Apr 9, 2025

tgross35 commented Apr 9, 2025

petrosagg commented Apr 9, 2025

RalfJung commented Apr 10, 2025 •

edited

Loading

petrosagg commented Apr 10, 2025

RalfJung commented Apr 10, 2025

petrosagg commented Apr 10, 2025

petrosagg commented Apr 10, 2025

rustbot commented Apr 11, 2025

petrosagg commented Apr 11, 2025

saethlin commented Apr 11, 2025 •

edited

Loading

petrosagg commented Apr 11, 2025

RalfJung Apr 11, 2025

petrosagg Apr 11, 2025

RalfJung Apr 11, 2025

petrosagg Apr 11, 2025

RalfJung Apr 14, 2025

petrosagg Apr 16, 2025

RalfJung commented Apr 17, 2025

bors commented Apr 17, 2025

ibraheemdev commented Apr 17, 2025

sync::mpsc: prevent double free on Drop #139553

sync::mpsc: prevent double free on Drop #139553

Conversation

petrosagg commented Apr 8, 2025

rustbot commented Apr 8, 2025

taiki-e commented Apr 8, 2025

tgross35 commented Apr 8, 2025

petrosagg commented Apr 9, 2025

petrosagg commented Apr 9, 2025

tgross35 commented Apr 9, 2025

petrosagg commented Apr 9, 2025

RalfJung commented Apr 10, 2025 • edited Loading

petrosagg commented Apr 10, 2025

RalfJung commented Apr 10, 2025

petrosagg commented Apr 10, 2025

petrosagg commented Apr 10, 2025

rustbot commented Apr 11, 2025

petrosagg commented Apr 11, 2025

saethlin commented Apr 11, 2025 • edited Loading

petrosagg commented Apr 11, 2025

RalfJung Apr 11, 2025

Choose a reason for hiding this comment

petrosagg Apr 11, 2025

Choose a reason for hiding this comment

RalfJung Apr 11, 2025

Choose a reason for hiding this comment

petrosagg Apr 11, 2025

Choose a reason for hiding this comment

RalfJung Apr 14, 2025

Choose a reason for hiding this comment

petrosagg Apr 16, 2025

Choose a reason for hiding this comment

RalfJung commented Apr 17, 2025

bors commented Apr 17, 2025

ibraheemdev commented Apr 17, 2025

sync::mpsc: prevent double free on `Drop` #139553

sync::mpsc: prevent double free on `Drop` #139553

RalfJung commented Apr 10, 2025 •

edited

Loading

saethlin commented Apr 11, 2025 •

edited

Loading